Skip to content

ReactiveElasticsearchClient should use the same request parameters as… #1703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 24, 2021
Merged

ReactiveElasticsearchClient should use the same request parameters as… #1703

merged 1 commit into from
Feb 24, 2021
+470 −56

Conversation

ffaoudi
Copy link
Contributor

@ffaoudi ffaoudi commented Feb 21, 2021 •
edited
Loading

… non reactive code

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our issue tracker.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 21, 2021
@ffaoudi ffaoudi force-pushed the 1658-ReactiveElasticsearchClient-should-use-the-same-request-parameters-as-non-reactive-code branch from 6d28592 to 2de7710 Compare February 21, 2021 21:28
@sothawo sothawo linked an issue Feb 22, 2021 that may be closed by this pull request
ReactiveElasticsearchClient should use the same request parameters as non-reactive code #1658
Closed
sothawo
sothawo requested changes Feb 22, 2021
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ReactiveElasticsearchClient I'd prefer if the deprecated versions use the fully qualified name of the parameter and the new functions use the imported names. Makes the old functions looking more ugly ;-9
Thank you for the PR, great work.

One missing piece: There are the existsIndex functions that use the old GetIndexRequest values which should be changed as well. The recently added getIndex calls already use the correct one.

Otherwise just some minor documentation things and one comment about which classes should be imported and which fully qualified

src/main/java/org/springframework/data/elasticsearch/client/reactive/RequestCreator.java Outdated
@@ -126,6 +126,10 @@
return RequestConverters::indexCreate;
}

default Function<org.elasticsearch.client.indices.CreateIndexRequest, Request> createIndex() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add @since 4.2; and we should deprecate the indexCreate() method that uses the old class.
It's a pity that we need a new name here. Although createIndex() is better than indexCreate in my opinion.
Or could use createIndexRequest() like you have it with the putMappingRequest, but then we probably should rename them all to xxxRequest(), deprecating the existing ones - might be something for a cleanup-ticket in the future.

But we should nevertheless deprecated the existing indexCreate method as it's only called by the deprectaed method from the client.

@ffaoudi ffaoudi force-pushed the 1658-ReactiveElasticsearchClient-should-use-the-same-request-parameters-as-non-reactive-code branch 2 times, most recently from be7d22d to 043f2b0 Compare February 22, 2021 23:37
sothawo
sothawo requested changes Feb 23, 2021
View reviewed changes
Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add versions of existsIndex() that use the "new" GetIndexRequest? That's all that's still missing

...g/springframework/data/elasticsearch/client/reactive/DefaultReactiveElasticsearchClient.java Outdated
@@ -653,6 +653,15 @@ private RequestBodySpec sendRequest(WebClient webClient, String logId, Request r
.next();
}

@Override
public Mono<Boolean> createIndex(HttpHeaders headers,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should have the imported class like in the interface, and the other one in line 649 the fully qualified name

...g/springframework/data/elasticsearch/client/reactive/DefaultReactiveElasticsearchClient.java Outdated
@@ -688,6 +697,14 @@ private RequestBodySpec sendRequest(WebClient webClient, String logId, Request r
return sendRequest(getMappingsRequest, requestCreator.getMapping(), GetMappingsResponse.class, headers).next();
}

@Override
public Mono<org.elasticsearch.client.indices.GetMappingsResponse> getMapping(HttpHeaders headers,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above with fully qualified names

...g/springframework/data/elasticsearch/client/reactive/DefaultReactiveElasticsearchClient.java Outdated
@@ -708,6 +725,14 @@ private RequestBodySpec sendRequest(WebClient webClient, String logId, Request r
.next();
}

@Override
public Mono<Boolean> putMapping(HttpHeaders headers,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@ffaoudi ffaoudi force-pushed the 1658-ReactiveElasticsearchClient-should-use-the-same-request-parameters-as-non-reactive-code branch from 043f2b0 to 15688da Compare February 23, 2021 22:48
@sothawo sothawo merged commit 3bc01a4 into spring-projects:master Feb 24, 2021
@sothawo
Copy link
Collaborator

sothawo commented Feb 24, 2021

Thanks again for you contribution. The PR has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sothawo sothawo sothawo approved these changes

Assignees
No one assigned
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ReactiveElasticsearchClient should use the same request parameters as non-reactive code
4 participants
@ffaoudi @sothawo @spring-projects-issues @faridfaoudi